Skip to content

add res to all nodes #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 17, 2024
Merged

add res to all nodes #97

merged 1 commit into from
Jul 17, 2024

Conversation

kousun12
Copy link
Contributor

previously if you you had a graph where B depends on A, and you did res = substrate.run(B), we would error if you tried to do res.get(A) even though it was in the response.

@kousun12 kousun12 requested a review from liamgriffiths July 17, 2024 05:10
@@ -93,10 +92,10 @@ export class Substrate {
* @throws {Error} when the client encounters an error making the request.
*/
async runSerialized(
serialized: any,
nodes: Node[] | null = null,
nodes: Node[],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really sure why this was optional before but seems like it's not used anywhere else, so it seemed okay to change to required

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure why it can also accept null there either, but that makes sense to me to remove that since we're always passing it through for now

Copy link
Contributor

@liamgriffiths liamgriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kousun12 looks good to me 👍

@kousun12 kousun12 merged commit 5a0f2cb into main Jul 17, 2024
2 checks passed
@kousun12 kousun12 deleted the rc-all-nodes branch July 17, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants